PR Reviewer Guide 🔍

(Review updated until commit 9491f5a)

Here are some key observations to aid the review process:

Dictionary<string, object?>? parameters = JsonSerializer.Deserialize<Dictionary<string, object?>>(value, CommandJsonSerializerContext.Default.DictionaryStringObject!);
return parameters;

return JsonSerializer.Serialize(this.Parameters, s_jsonSerializerOptions);

PR Code Suggestions ✨

Latest suggestions up to 9491f5a
Explore these optional code suggestions:

-Dictionary<string, object?>? parameters = JsonSerializer.Deserialize<Dictionary<string, object?>>(value, s_jsonSerializerOptions);
+Dictionary<string, object?>? parameters = JsonSerializer.Deserialize<Dictionary<string, object?>>(value, CommandJsonSerializerContext.Default.DictionaryStringObject);

Previous suggestions

 private static Dictionary<string, object?>? ConvertParametersFromJson(string value)
 {
+    Dictionary<string, object?>? parameters = JsonSerializer.Deserialize<Dictionary<string, object?>>(value, CommandJsonSerializerContext.Default.DictionaryStringObject!);
     return parameters;
 }

 private static Dictionary<string, object?>? ConvertParametersFromJson(string value)
 {
-    Dictionary<string, object?>? parameters = JsonSerializer.Deserialize<Dictionary<string, object?>>(value, CommandJsonSerializerContext.Default.DictionaryStringObject!);
-    return parameters;
+    return JsonSerializer.Deserialize<Dictionary<string, object?>>(value, CommandJsonSerializerContext.Default.DictionaryStringObject!);
 }

With this PR, there are only 4 AOT warnings remaining (really two locations, coming from one single issue: the best-effort AOT support for Command.ParametersAsJsonString

I see 2 options:

• Mark all enums with [CamelCaseJsonEnumStringConverter] attribute
• Add all enums (one by one) explicitly for BiDiJsonSerializerContext

I am not sure which way is better. I propose to hold on it until #15329 is resolved. Then we will understand how to "extend" serializer context, and will apply more accurate decision here.

OK, I will break off the enum string changes until a decision is made there. Everything else should still be valid.

Only question this PR brought up: JsonStringEnumConverter<TEnum> on each enum, or BiDi-wide JsonStringEnumConverter on the options?

I removed those changes from this PR. Everything else should be straightforward.

If we merge this PR, we can enable AOT warnings on the project as a whole, making AOT questions much simpler.

Using the overload with JsonTypeInfo helped. Thanks all..